Skip to content

[wasmfs] Make standalone's open syscall weak, to not conflict with WASMFS - #15992

Merged
kripken merged 1 commit into
mainfrom
wfstand
Jan 12, 2022
Merged

[wasmfs] Make standalone's open syscall weak, to not conflict with WASMFS#15992
kripken merged 1 commit into
mainfrom
wfstand

Conversation

@kripken

@kripken kripken commented Jan 12, 2022

Copy link
Copy Markdown
Member

Both libwasmfs and libstandalonewasm implement the open syscall, as well as
others (only the first is in this PR). I think we should make them weak in
standalone wasm, that is, if wasmfs is present we always want to run that code.
The wasmfs syscall will then call lower-level things which we will need to implement
in standalone wasm (like we do in JS).

@kripken
kripken requested review from sbc100 and tlively January 12, 2022 22:41

@tlively tlively left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to add a test showing that this works?

@kripken

kripken commented Jan 12, 2022

Copy link
Copy Markdown
Member Author

Not yet. Plenty more broken before the first standalone+wasmfs test passes...

@sbc100

sbc100 commented Jan 12, 2022

Copy link
Copy Markdown
Collaborator

Presumably this would not be problem is we spit libstandalone in to single source file per function?

(Except in the case of --whole-archive .. which we do use in -sMAIN_MODULE=1 mode.. but that mode does't work with standlone wasm ayway).

@kripken

kripken commented Jan 12, 2022

Copy link
Copy Markdown
Member Author

We could also split libstandalone into one file per function, yes, if we also made sure to link in wasmfs first and not later. That seems more complicated, though, to me.

@sbc100

sbc100 commented Jan 12, 2022

Copy link
Copy Markdown
Collaborator

I didn't mean to say we shouldn't land this.

I think it might be common for folks to want to replace these stub functions so going forward I think splitting up the library could make more sense than marking each of the function in it as weak. Its a fairly common pattern to override a stdlib function by including your own version of it, and the way that libc and other core libraries allow this is by putting each function it its own source file.

This change LGTM though.

@kripken

kripken commented Jan 12, 2022

Copy link
Copy Markdown
Member Author

Sounds good. I do agree that separately splitting up libstandalonewasm is good for the reasons you mentioned.

@kripken

kripken commented Jan 12, 2022

Copy link
Copy Markdown
Member Author

Landing this to unblock progress with that LGTM. We can change this later if we want. In fact, once wasmfs is the default we could completely erase the syscalls in libstandalonewasm...

@kripken
kripken merged commit f5482c4 into main Jan 12, 2022
@kripken
kripken deleted the wfstand branch January 12, 2022 23:48
@kripken

kripken commented Jan 13, 2022

Copy link
Copy Markdown
Member Author

Actually, I'm not sure how wasmfs could work with standalone wasm. Wasmfs defines wasi syscalls. What I'd like to do is for wasmfs to intercept those syscalls, then handle them itself if it should - MemFS, in particular - but when going for stdout/stderr we should just forward to the VM's wasi syscalls. So we want wasmfs to "interpose" here. Is that possible (without JS)?

@sbc100

sbc100 commented Jan 13, 2022

Copy link
Copy Markdown
Collaborator

Yes I think we can do it. We could define __syscall_foo in libstandalone.a and then in standalone.c we can import the same syscall under a different name.

e.g.:

__wasi_errno_t __wasmfs_path_open(__attribute__((                                                                
    __import_module__("wasi_snapshot_preview1"),                                                         
    __import_name__("path_open"));

This would be an alternative import to one defined in wasi/api.h. Two different C symbols referring to the same import. One of those C symbols would not end up resulting in an actual import because libstandalone.a would define it.

@kripken

kripken commented Jan 13, 2022

Copy link
Copy Markdown
Member Author

I see, thanks @sbc100 I opened #16008 for that now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants